Hi @lukalt!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

Hey @lukalt Thanks for the contribution. Could you please sign the CLA?

Hey @lukalt Thanks for the contribution. Could you please sign the CLA?

The signed CLA will be submitted beginning of next week. Sorry for the delay

Hey,

@praihan The CLA has been submitted now
@vitaut Thank you for the feedback. I absolutely agree and made the new parameter an optional named argument of that macro.

I also integrated the Protocol tests into the CMake project. There seems to be some incompatibility between gtest and glog causing CHECK(..) calls inside EXPECTED_DEATH to fail with the error macro "EXPECT_EXIT" passed 4 arguments, but takes just 3. I rewrote the failing lines to not use glog for now.

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

Thank you. I made the final change now. Builds on Windows seem to be failing but it does not look like it is caused by my changes. It looks like it is related to fmtlib

@vitaut has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@vitaut has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@vitaut Formatting issues and test failures are now resolved

@vitaut Formatting issues and test failures are now resolved

Great but please update the copyright headers per my earlier comment.

@vitaut Copyright note has been updated now

@vitaut has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

There are still test failures:

[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from BinaryProtocolTest
[ RUN      ] BinaryProtocolTest.writeInvalidBool
fbcode/thrift/lib/cpp2/protocol/test/BinaryProtocolTest.cpp:63: Failure
Death test: { w.writeBool(makeInvalidBool()); auto s = std::string(); q.appendToString(s); if (s == std::string(1, '\0')) { exit(1); } }
    Result: died but not with expected error.
  Expected: contains regular expression "invalid bool value"
Actual msg:
[  DEATH   ] 

[  FAILED  ] BinaryProtocolTest.writeInvalidBool (58 ms)

There are still test failures:

[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from BinaryProtocolTest
[ RUN      ] BinaryProtocolTest.writeInvalidBool
fbcode/thrift/lib/cpp2/protocol/test/BinaryProtocolTest.cpp:63: Failure
Death test: { w.writeBool(makeInvalidBool()); auto s = std::string(); q.appendToString(s); if (s == std::string(1, '\0')) { exit(1); } }
    Result: died but not with expected error.
  Expected: contains regular expression "invalid bool value"
Actual msg:
[  DEATH   ] 

[  FAILED  ] BinaryProtocolTest.writeInvalidBool (58 ms)

You run the tests on x86, right? I was running them on arm64 and got no failures, probably because char is unsigned on aarch64. I will see how we can fix this test case

You run the tests on x86, right?

Yes, the failures are on x86_64.

Test failure should be fixed. I tested it on a x86 system with gcc 13.1.0.

One question regarding writeBool for the compact protocol though. Is it acceptable if the function does not throw an error when a malformed bool is provided as an argument and instead writes a valid bool to the output. This is how I understand the purpose of this test.